Add missing resolveUri method to FlysystemStorage #1441

sovetski · 2024-04-07T12:41:02Z

It resolves a lot of problems related to Flysystem.

Without this PR, if we are using S3, Cloudflare images or other cloud storage, the public url is not the CDN one but a local url which is incorrect.

For the people who will suggest using uri_prefix, as I mentionned here: #1434 (comment)
In my case it is impossible to add uri_prefix manually, because in Cloudflare, the file name is not at the end, it is something like https://imagedelivery.net/qsdazd54azd98a4z69d5/image3.jpg/public the /public at the end makes it impossible because it is not just a prefix, we have to do some sprintf etc.

The problem existe since more than 8 years, I think it is time to automatise it with this PR.

I hope that it will be ok for maintainers.

garak

please add a test for the new method

sovetski · 2024-04-10T18:22:58Z

@garak should be fine now

alcohol · 2024-04-12T08:30:24Z

This change broke our whole implementation. Why was this not done in a major release?

We're using flysystem as our storage backend.

Before this change, everything resolved fine, e.g. to a URI like:

https://bucket.ams3.digitaloceanspaces.com/media/cache/filter/subdir/c5b74a86da9c6a4e4c7132a2be83d3f43c9cade7.jpeg

After this change the following was resolved:

https://bucket.ams3.digitaloceanspaces.com/media/cache/filter/https---bucket.ams3.digitaloceanspaces.com/subdir/4ea7859db76e4befaf27ad97864ac36b4976e29d.jpg

We are using the helper to get the path, and then attempt to read it using our flysystem cache adapter, e.g.

$path = $this->helper->asset($entity);
$content = $this->defaultStorage->read($path);

sovetski · 2024-04-12T08:37:59Z

Hi @alcohol, what does your helper exactly? Can you put the code here? The problem can also be related to your custom adapter, do you have all required methods implemented? If you can provide your code here, it will be easier to debug it and see the problem.

alcohol · 2024-04-12T08:42:35Z

The helper is simply an instance of Vich\UploaderBundle\Templating\Helper\UploaderHelper.

Instead of returning a path it is now returning a full uri.

sovetski · 2024-04-12T08:44:44Z

@alcohol if in your helper, you change the "resolveUri" by "resolvePath" does it work?

alcohol · 2024-04-12T08:45:13Z

Our cached S3/Flysystem adapter

<?php declare(strict_types=1);

namespace redacted\Flysystem;

use DateTimeInterface;
use League\Flysystem\CalculateChecksumFromStream;
use League\Flysystem\ChecksumAlgoIsNotSupported;
use League\Flysystem\ChecksumProvider;
use League\Flysystem\Config;
use League\Flysystem\DirectoryAttributes;
use League\Flysystem\FileAttributes;
use League\Flysystem\FilesystemAdapter;
use League\Flysystem\StorageAttributes;
use League\Flysystem\UnableToCopyFile;
use League\Flysystem\UnableToMoveFile;
use League\Flysystem\UnableToProvideChecksum;
use League\Flysystem\UnableToReadFile;
use League\Flysystem\UnableToRetrieveMetadata;
use League\Flysystem\UrlGeneration\PublicUrlGenerator;
use League\Flysystem\UrlGeneration\TemporaryUrlGenerator;
use Psr\Cache\CacheItemPoolInterface;
use RuntimeException;

class CacheAdapter implements FilesystemAdapter, ChecksumProvider, PublicUrlGenerator, TemporaryUrlGenerator
{
    use CacheItemsTrait;
    use CalculateChecksumFromStream;

    public function __construct(
        private readonly FilesystemAdapter $adapter,
        private readonly CacheItemPoolInterface $cache,
        private readonly string $cacheKeyPrefix = 'flysystem_',
    ) {}

    public function fileExists(string $path): bool
    {
        $item = $this->getCacheItem($path);

        if ($item->isHit()) {
            return $item->get() instanceof FileAttributes;
        }

        if ( ! $this->adapter->fileExists($path)) {
            return false;
        }

        $item->set(new FileAttributes(path: $path));

        $this->saveCacheItem($item);

        return true;
    }

    public function directoryExists(string $path): bool
    {
        $item = $this->getCacheItem($path);

        if ($item->isHit()) {
            return $item->get() instanceof DirectoryAttributes;
        }

        if ( ! $this->adapter->directoryExists($path)) {
            return false;
        }

        $item->set(new DirectoryAttributes(path: $path));

        $this->saveCacheItem($item);

        return true;
    }

    public function write(string $path, string $contents, Config $config): void
    {
        $this->adapter->write($path, $contents, $config);

        $this->addCacheEntry($path, new FileAttributes($path));
    }

    public function writeStream(string $path, $contents, Config $config): void
    {
        $this->adapter->writeStream($path, $contents, $config);

        $this->addCacheEntry($path, new FileAttributes($path));
    }

    public function read(string $path): string
    {
        try {
            $contents = $this->adapter->read($path);
        } catch (UnableToReadFile $e) {
            $this->purgeCacheItem($path);

            throw $e;
        }

        $item = $this->getCacheItem($path);

        if ( ! $item->isHit()) {
            $fileAttributes = new FileAttributes(path: $path);

            $item->set($fileAttributes);

            $this->saveCacheItem($item);
        }

        return $contents;
    }

    public function readStream(string $path)
    {
        try {
            $resource = $this->adapter->readStream($path);
        } catch (UnableToReadFile $e) {
            $this->purgeCacheItem($path);

            throw $e;
        }

        $item = $this->getCacheItem($path);

        if ( ! $item->isHit()) {
            $fileAttributes = new FileAttributes(path: $path);

            $item->set($fileAttributes);

            $this->saveCacheItem($item);
        }

        return $resource;
    }

    public function delete(string $path): void
    {
        try {
            $this->adapter->delete($path);
        } finally {
            $this->purgeCacheItem($path);
        }
    }

    public function deleteDirectory(string $path): void
    {
        try {
            foreach ($this->adapter->listContents($path, true) as $storageAttributes) {
                /** @var StorageAttributes $storageAttributes */
                $this->purgeCacheItem($storageAttributes->path());
            }

            $this->adapter->deleteDirectory($path);
        } finally {
            $this->purgeCacheItem($path);
        }
    }

    public function createDirectory(string $path, Config $config): void
    {
        $this->adapter->createDirectory($path, $config);

        $this->addCacheEntry($path, new DirectoryAttributes(path: $path));
    }

    public function setVisibility(string $path, string $visibility): void
    {
        $this->adapter->setVisibility($path, $visibility);

        $item = $this->getCacheItem($path);

        if ($item->isHit()) {
            $currentAttributes = $item->get();

            if ($currentAttributes instanceof FileAttributes) {
                $additionalAttributes = new FileAttributes(path: $path, visibility: $visibility);
            } elseif ($currentAttributes instanceof DirectoryAttributes) {
                $additionalAttributes = new DirectoryAttributes(path: $path, visibility: $visibility);
            } else {
                throw new RuntimeException('Cached item is not a file or a directory.');
            }

            $item->set($this->mergeAttributes($currentAttributes, $additionalAttributes));

            $this->saveCacheItem($item);
        }

        // We cannot create the cache item if it does not exist since we don't know if it's a file or a directory.
    }

    public function visibility(string $path): FileAttributes
    {
        try {
            return $this->getFileAttributes(
                path: $path,
                loader: fn (): FileAttributes => $this->adapter->visibility($path),
                accessor: static fn (FileAttributes $fileAttributes): ?string => $fileAttributes->visibility(),
            );
        } catch (RuntimeException $previous) {
            throw UnableToRetrieveMetadata::visibility($path, $previous->getMessage(), $previous);
        }
    }

    public function mimeType(string $path): FileAttributes
    {
        try {
            return $this->getFileAttributes(
                path: $path,
                loader: fn (): FileAttributes => $this->adapter->mimeType($path),
                accessor: static fn (FileAttributes $fileAttributes): ?string => $fileAttributes->mimeType(),
            );
        } catch (RuntimeException $previous) {
            throw UnableToRetrieveMetadata::mimeType($path, $previous->getMessage(), $previous);
        }
    }

    public function lastModified(string $path): FileAttributes
    {
        try {
            return $this->getFileAttributes(
                path: $path,
                loader: fn (): FileAttributes => $this->adapter->lastModified($path),
                accessor: static fn (FileAttributes $fileAttributes): ?int => $fileAttributes->lastModified(),
            );
        } catch (RuntimeException $previous) {
            throw UnableToRetrieveMetadata::lastModified($path, $previous->getMessage(), $previous);
        }
    }

    public function fileSize(string $path): FileAttributes
    {
        try {
            return $this->getFileAttributes(
                path: $path,
                loader: fn (): FileAttributes => $this->adapter->fileSize($path),
                accessor: static fn (FileAttributes $fileAttributes): ?int => $fileAttributes->fileSize(),
            );
        } catch (RuntimeException $previous) {
            throw UnableToRetrieveMetadata::fileSize($path, $previous->getMessage(), $previous);
        }
    }

    public function checksum(string $path, Config $config): string
    {
        $algo = $config->get('checksum_algo');
        $metadataKey = isset($algo) ? 'checksum_'.$algo : 'checksum';

        $attributeAccessor = function (StorageAttributes $storageAttributes) use ($metadataKey) {
            if (is_a($this->adapter, 'League\Flysystem\AwsS3V3\AwsS3V3Adapter')) {
                // Special optimization for AWS S3, but won't break if adapter not installed
                $etag = $storageAttributes->extraMetadata()['ETag'] ?? null;

                if (isset($etag)) {
                    $checksum = trim($etag, '" ');
                }
            }

            return $checksum ?? $storageAttributes->extraMetadata()[$metadataKey] ?? null;
        };

        try {
            $fileAttributes = $this->getFileAttributes(
                path: $path,
                loader: function () use ($path, $config, $metadataKey): FileAttributes {
                    // This part is "mirrored" from FileSystem class to provide the fallback mechanism and be able to cache the result
                    try {
                        if ( ! $this->adapter instanceof ChecksumProvider) {
                            throw new ChecksumAlgoIsNotSupported();
                        }

                        $checksum = $this->adapter->checksum($path, $config);
                    } catch (ChecksumAlgoIsNotSupported) {
                        $checksum = $this->calculateChecksumFromStream($path, $config);
                    }

                    return new FileAttributes(path: $path, extraMetadata: [$metadataKey => $checksum]);
                },
                accessor: $attributeAccessor
            );
        } catch (RuntimeException $e) {
            throw new UnableToProvideChecksum($e->getMessage(), $path, $e);
        }

        return $attributeAccessor($fileAttributes);
    }

    public function listContents(string $path, bool $deep): iterable
    {
        foreach ($this->adapter->listContents($path, $deep) as $storageAttributes) {
            $item = $this->getCacheItem($storageAttributes->path());

            if ($item->isHit()) {
                $cachedStorageAttributes = $item->get();

                if ($cachedStorageAttributes instanceof FileAttributes && $storageAttributes instanceof FileAttributes) {
                    $cachedStorageAttributes = $this->mergeAttributes(current: $cachedStorageAttributes, additional: $storageAttributes);
                } elseif ($cachedStorageAttributes instanceof DirectoryAttributes && $storageAttributes instanceof DirectoryAttributes) {
                    $cachedStorageAttributes = $this->mergeAttributes(current: $cachedStorageAttributes, additional: $storageAttributes);
                }
            } else {
                $cachedStorageAttributes = null;
            }

            $item->set($cachedStorageAttributes ?? $storageAttributes);

            $this->saveCacheItem($item);

            yield $storageAttributes;
        }
    }

    public function move(string $source, string $destination, Config $config): void
    {
        try {
            $this->adapter->move($source, $destination, $config);
        } catch (UnableToMoveFile $e) {
            $this->purgeCacheItem($source);
            $this->purgeCacheItem($destination);

            throw $e;
        }

        $itemSource = $this->getCacheItem($source);
        $itemDestination = $this->getCacheItem($destination);

        if ($itemSource->isHit()) {
            /** @var StorageAttributes $sourceStorageAttributes */
            $sourceStorageAttributes = $itemSource->get();

            $destinationStorageAttributes = $sourceStorageAttributes->withPath($destination);

            $this->deleteCacheItem($itemSource);
        } else {
            $destinationStorageAttributes = new FileAttributes(path: $destination);
        }

        $itemDestination->set($destinationStorageAttributes);

        $this->saveCacheItem($itemDestination);
    }

    public function copy(string $source, string $destination, Config $config): void
    {
        try {
            $this->adapter->copy($source, $destination, $config);
        } catch (UnableToCopyFile $e) {
            $this->purgeCacheItem($source);
            $this->purgeCacheItem($destination);

            throw $e;
        }

        $itemSource = $this->getCacheItem($source);
        $itemDestination = $this->getCacheItem($destination);

        if ($itemSource->isHit()) {
            /** @var StorageAttributes $sourceStorageAttributes */
            $sourceStorageAttributes = $itemSource->get();

            $destinationStorageAttributes = $sourceStorageAttributes->withPath($destination);
        } else {
            $destinationStorageAttributes = new FileAttributes(path: $destination);
        }

        $itemDestination->set($destinationStorageAttributes);

        $this->saveCacheItem($itemDestination);
    }

    public function publicUrl(string $path, Config $config): string
    {
        if ( ! $this->adapter instanceof PublicUrlGenerator) {
            throw new RuntimeException('The adapter does not support public URLs.');
        }

        return $this->adapter->publicUrl($path, $config);
    }

    public function temporaryUrl(string $path, DateTimeInterface $expiresAt, Config $config): string
    {
        if ( ! $this->adapter instanceof TemporaryUrlGenerator) {
            throw new RuntimeException('The adapter does not support temporary URLs.');
        }

        return $this->adapter->temporaryUrl($path, $expiresAt, $config);
    }
}

alcohol · 2024-04-12T08:48:54Z

@alcohol if in your helper, you change the "resolveUri" by "resolvePath" does it work?

It is not my helper, it is vendor code from this library. Previously we were getting $mapping->getUriPrefix().'/'.$path;, now we are getting the $fs->publicUrl($path);

sovetski · 2024-04-12T08:52:06Z

@alcohol if in your helper, you change the "resolveUri" by "resolvePath" does it work?

It is not my helper, it is vendor code from this library. Previously we were getting $mapping->getUriPrefix().'/'.$path;, now we are getting the $fs->publicUrl($path);

As I am not able to do it at the moment, you can try to edit the vendor's code just to see what happens and put the original code back? I am not using S3 or this helper, that is why I need more details from your side if possible, to be able to find a fix

alcohol · 2024-04-12T08:53:49Z

The behaviour documented here has changed if you use the FlysystemStorage implementation: https://github.com/dustin10/VichUploaderBundle/blob/master/docs/generating_urls.md#generating-a-url-in-a-controller

I am not saying this change is bad, but it drastically differs from what is documented and was previously default behaviour. The FlysystemStorage storage was falling back to the resolveUri method from the AbstractStorage implementation. By overriding that method the returned value can be very different suddenly. While this might be considered a "bugfix", I feel such a major change should have been done in a new major version.

Cryde · 2024-04-14T13:24:45Z

Hello

I had the same issue, I guess bundle like https://github.com/1up-lab/OneupFlysystemBundle/tree/main are not "ready" for this change because I can't see where we can configure public urls (https://flysystem.thephpleague.com/docs/usage/public-urls/)
Or maybe I miss something ?

garak · 2024-04-14T13:43:58Z

Never used flysystem myself, please let me know if I need to revert this change

sovetski · 2024-04-14T14:06:12Z

@Cryde I don't know if it can help, but for Flysystem, I am using this package and it works as expected: thephpleague/flysystem-bundle, documentation: https://github.com/dustin10/VichUploaderBundle/blob/master/docs/storage/flysystem.md#integrating-with-thephpleagueflysystem-bundle

@garak as mentionned @alcohol, maybe you can put it in a major release instead?

garak · 2024-04-14T14:09:39Z

I could put it in a major, but a major release is not currently planned

andyexeter · 2024-04-19T08:47:05Z

This update broke things for us too. We are storing images in S3 using a Cloudfront URL as a uri_prefix. The uri_prefix config is now completely ignored and the S3 URL is returned when using the \Vich\UploaderBundle\Templating\Helper\UploaderHelper::asset method.

sovetski · 2024-04-19T09:04:21Z

This update broke things for us too. We are storing images in S3 using a Cloudfront URL as a uri_prefix. The uri_prefix config is now completely ignored and the S3 URL is returned when using the \Vich\UploaderBundle\Templating\Helper\UploaderHelper::asset method.

It solves a problem that shouldn't be there in the first place. I'm not opposed to reverting this change, but do you have a better approach to suggest?

Reverting is just a way to put the problem back; I find suggesting an improvement more interesting than wiping everything out at once.

If everyone agrees to test, I can submit a new pull request to fix the issue for existing projects. But again, just because it works with a makeshift solution doesn't mean it should never evolve (in my opinion)

garak · 2024-04-19T09:09:55Z

What about adding an alternate storage? We need to avoid breaking existing implementations

andyexeter · 2024-04-19T09:21:23Z

I understand that it solves a problem, but it does so by breaking existing functionality.

Forgive me if I'm misunderstanding, but looking at this code:

VichUploaderBundle/src/Storage/FlysystemStorage.php

Lines 121 to 125 in fec8c14

    
           try { 
        
               return $fs->publicUrl($path); 
        
           } catch (FilesystemException) { 
        
               return $mapping->getUriPrefix().'/'.$path; 
        
           }

The code within the try block completely ignores the uri_prefix config, doesn't it? So it breaks for anyone using any uri_prefix?

sovetski · 2024-04-19T09:27:17Z

@andyexeter yes you are right, it prioritizes the publicUrl instead of uri_prefix. As @garak said, we should avoid to break existing implementations, I will do a PR to revert changes, and maybe in the future it will be implemented as a major change

sovetski · 2024-04-19T09:31:45Z

@garak the PR is ready to rollback changes: #1444

sovetski added 3 commits April 7, 2024 14:38

Add missing resolveUri method to FlysystemStorage

34193d2

Fix indentations

e41eedb

Fix the error when publicUrl is not available

695ad53

garak requested changes Apr 8, 2024

View reviewed changes

Add test for resolveUri method in FlysystemStorage

fec8c14

garak approved these changes Apr 10, 2024

View reviewed changes

garak merged commit ec9ad4b into dustin10:master Apr 11, 2024
9 of 10 checks passed

sovetski added a commit to sovetski/VichUploaderBundle that referenced this pull request Apr 19, 2024

Revert changes of dustin10#1441

38e4c24

sovetski mentioned this pull request Apr 19, 2024

Revert changes of #1441 #1444

Merged

garak pushed a commit that referenced this pull request Apr 23, 2024

Revert changes of #1441 (#1444)

0ed4538

dheineman mentioned this pull request May 17, 2024

Optionally resolve the uri through flysystem #1451

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add missing resolveUri method to FlysystemStorage #1441

Add missing resolveUri method to FlysystemStorage #1441

sovetski commented Apr 7, 2024 •

edited

Loading

garak left a comment

sovetski commented Apr 10, 2024

alcohol commented Apr 12, 2024 •

edited

Loading

sovetski commented Apr 12, 2024 •

edited

Loading

alcohol commented Apr 12, 2024

sovetski commented Apr 12, 2024

alcohol commented Apr 12, 2024 •

edited

Loading

alcohol commented Apr 12, 2024

sovetski commented Apr 12, 2024

alcohol commented Apr 12, 2024 •

edited

Loading

Cryde commented Apr 14, 2024

garak commented Apr 14, 2024

sovetski commented Apr 14, 2024 •

edited

Loading

garak commented Apr 14, 2024

andyexeter commented Apr 19, 2024 •

edited

Loading

sovetski commented Apr 19, 2024

garak commented Apr 19, 2024

andyexeter commented Apr 19, 2024

sovetski commented Apr 19, 2024

sovetski commented Apr 19, 2024

Add missing resolveUri method to FlysystemStorage #1441

Add missing resolveUri method to FlysystemStorage #1441

Conversation

sovetski commented Apr 7, 2024 • edited Loading

garak left a comment

Choose a reason for hiding this comment

sovetski commented Apr 10, 2024

alcohol commented Apr 12, 2024 • edited Loading

sovetski commented Apr 12, 2024 • edited Loading

alcohol commented Apr 12, 2024

sovetski commented Apr 12, 2024

alcohol commented Apr 12, 2024 • edited Loading

alcohol commented Apr 12, 2024

sovetski commented Apr 12, 2024

alcohol commented Apr 12, 2024 • edited Loading

Cryde commented Apr 14, 2024

garak commented Apr 14, 2024

sovetski commented Apr 14, 2024 • edited Loading

garak commented Apr 14, 2024

andyexeter commented Apr 19, 2024 • edited Loading

sovetski commented Apr 19, 2024

garak commented Apr 19, 2024

andyexeter commented Apr 19, 2024

sovetski commented Apr 19, 2024

sovetski commented Apr 19, 2024

sovetski commented Apr 7, 2024 •

edited

Loading

alcohol commented Apr 12, 2024 •

edited

Loading

sovetski commented Apr 12, 2024 •

edited

Loading

alcohol commented Apr 12, 2024 •

edited

Loading

alcohol commented Apr 12, 2024 •

edited

Loading

sovetski commented Apr 14, 2024 •

edited

Loading

andyexeter commented Apr 19, 2024 •

edited

Loading